-
Notifications
You must be signed in to change notification settings - Fork 1.4k
SEP-1036: URL Elicitation #1105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SEP-1036: URL Elicitation #1105
Conversation
|
Current status: ✅ Spec type checks and tests are passing |
commit: |
| if (request.params.mode !== 'form') { | ||
| throw new McpError(ErrorCode.InvalidParams, `Unsupported elicitation mode: ${request.params.mode}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is technically redundant, since the base client already validates the mode before invoking this handler. However, the request handler is typed to ElicitRequestSchema because there is not an ElicitFormRequestSchema (there is an ElicitRequestFormParamsSchema, but that is an inner shape). So without this code checking for mode="form", TypeScript complains because it is not 100% sure if the payload is of the right type.
I didn't want to boil the ocean and change too much of the contract of client.setRequestHandler(ElicitRequestSchema...), but please give me feedback if you think I should change this further!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, we could technically achieve setting a request handler by having 2 more schemas ElicitRequestURLSchema, ElicitRequestFormSchema, but I think that locks us down further by introducing new public interfaces.
While the client does check this under the hood out of the box, I think it doesn't hurt too much for the end user to be aware that there are multiple modes and to be having to write some checks/guardrails their side about the mode and generally have awareness of how they handle each mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good w/ me!
0593256 to
4b4516f
Compare
|
Now the only remaining breaking change can be seen in: https://github.com/modelcontextprotocol/typescript-sdk/pull/1105/files/237f84c6b168cbc74a4bc3836846a915654df390#diff-629b7a15c09a4a699f9f49eac4d4768c03b1c42e0856c687c827ed000e9171eeR235-R238 Previously a client using the SDK could do client.setRequestHandler(ElicitRequestSchema, async request => {
console.log('\n🔔 Elicitation Request Received:');
// ...But now because client.setRequestHandler(ElicitRequestSchema, async request => {
if (request.params.mode !== 'form') {
throw new McpError(ErrorCode.InvalidParams, `Unsupported elicitation mode: ${request.params.mode}`);
}
console.log('\n🔔 Elicitation (form) Request Received:');
// ...otherwise I get: @KKonstantinov Do you want me to try some additional shenanigans to make existing code that calls |
| if (request.params.mode !== 'form') { | ||
| throw new McpError(ErrorCode.InvalidParams, `Unsupported elicitation mode: ${request.params.mode}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, we could technically achieve setting a request handler by having 2 more schemas ElicitRequestURLSchema, ElicitRequestFormSchema, but I think that locks us down further by introducing new public interfaces.
While the client does check this under the hood out of the box, I think it doesn't hurt too much for the end user to be aware that there are multiple modes and to be having to write some checks/guardrails their side about the mode and generally have awareness of how they handle each mode.
|
Thank you for your work on this! Have left one comment about LGTM! |
|
@KKonstantinov Done, thanks for your expert guidance! Looks like I need a review from modelcontextprotocol/typescript-sdk-auth in order to merge. |
felixweinberger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this SEP & accompanying implementation!
Mostly nits on the readme, but one higher level piece of feedback I'd like to discuss.
It looks like on the server-side we implement 2 different methods and types to support this, one for form, one for url.
On the client on the other hand we rely on the mode param to discriminate between the two with a single handler.
This asymmetry seems less than ideal - did we have to do it this way? Was it not possible to discriminate between the modes on the server with the mode field for some reason?
Having chatted with @KKonstantinov I understand the main reason for this choice was to avoid a backwards incompatible change right? Changing the type signature of Would it be possible to resolve this via an |
|
@felixweinberger @cliffhall Thanks for your comments! I've restored a single Existing code that calls |
| * Creates an elicitation request for the given parameters. | ||
| * @param params The parameters for the form elicitation request (legacy signature without mode). | ||
| * @param options Optional request options. | ||
| * @returns The result of the elicitation request. | ||
| */ | ||
| async elicitUrl(params: Omit<ElicitRequestURLParams, 'mode'>, options?: RequestOptions): Promise<ElicitResult> { | ||
| const mode = 'url'; | ||
| if (!this._clientCapabilities?.elicitation?.[mode]) { | ||
| throw new Error(`Client does not support ${mode} elicitation.`); | ||
| async elicitInput(params: LegacyElicitRequestFormParams, options?: RequestOptions): Promise<ElicitResult>; | ||
| async elicitInput( | ||
| params: LegacyElicitRequestFormParams | ElicitRequestFormParams | ElicitRequestURLParams, | ||
| options?: RequestOptions | ||
| ): Promise<ElicitResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we could do something like this to mark specifically the Legacy version as @deprecated. But we can do that in a follow-up as well.
... other elicitation implementations
/**
* Creates an elicitation request for the given parameters.
* @deprecated Use the overload with explicit `mode: 'form'` instead.
* @param params The parameters for the form elicitation request (legacy signature without mode).
*/
async elicitInput(params: LegacyElicitRequestFormParams, options?: RequestOptions): Promise<ElicitResult>;
// Implementation signature (not visible to callers)
async elicitInput(
params: LegacyElicitRequestFormParams | ElicitRequestFormParams | ElicitRequestURLParams,
options?: RequestOptions
): Promise<ElicitResult> {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'm happy to do a quick follow-up after this. I can also capture what ends up landing in modelcontextprotocol/modelcontextprotocol#1825
Thanks @nbarbettini for the quick turnaround! Requesting review from @pcarleton as |
|
@nbarbettini heads-up: you'll have to remove the |
|
@cliffhall I don't think I am using Edit: Oh were you referring to the follow-up suggestion from @felixweinberger? If I am understanding right, we should not add new |
Hey @cliffhall thanks for pointing that out. I don't think the discussion in that SEP automatically extends to SDKs. Our approach on the protocol right now is to not "deprecate" and just name things as "legacy". We don't actually call any protocol types "deprecated" here - we're just considering using the For example, there are already multiple uses of |
|
Thanks all! @felixweinberger Makes sense about |
|
@nbarbettini @felixweinberger when I run the Error running MCP client: InvalidRequestError: [
{
"code": "invalid_type",
"expected": "object",
"received": "undefined",
"path": [],
"message": "Required"
}
]
Are you able to see it successfully run? Can you screenshot that? |
|
@cliffhall I might have regressed something after my last bit of refactoring. I will check today! |
|
Fix here: #1136 |
Thanks for catching this @cliffhall |


Motivation and Context
Closes #919
Implements SEP-1036, coming in the next version of MCP.
How Has This Been Tested?
Breaking Changes
Yes (only for existing elicitation implementations):
client.setRequestHandler(ElicitRequestSchema, () => ...)may need to add an additional check on theparams.modeparameter to keep TS happy:Types of changes
Checklist
Additional context
I organized the PR by commit, so hopefully it is easy to follow!
I will add some review notes inline in the code below.